Preserve pointer identity for float NaNs - #1144
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughFloat NaNs now use address identity and remain boxed or unspecialized where required. Complex values use pointer identity. Float-list matching uses direct equality. Tuple and list specializations exclude NaNs and float subclasses while finite exact floats retain specialized storage paths. ChangesIdentity semantics
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant PythonObject
participant FloatStrategy
participant JITTrace
participant BoxedStorage
PythonObject->>FloatStrategy: classify float value
FloatStrategy-->>JITTrace: accept finite exact float
JITTrace->>JITTrace: apply exact-class and non-NaN guards
JITTrace->>BoxedStorage: store NaN or subclass as original object
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 7cbda57). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patchNone. 3. Pre-existing mismatches (already present before this patch)None. 4. Structural adaptations
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7cbda573ea
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if one.is_nan() || two.is_nan() { | ||
| return false; |
There was a problem hiding this comment.
Keep NaNs out of all unboxed float storage
Once NaNs fall back to pointer identity here, every path that erases the original W_FloatObject has to reject them, not just FloatListStrategy. makespecialisedtuple2 still builds W_SpecialisedTupleObject_ff for exact NaNs and mapdict still picks UnboxType::Float; both store only the raw f64 and rebox on read, so cases like n = float('nan'); t = (n, n); t[0] is n or a NaN instance attribute now become false / get fresh id() values even though Python attribute and tuple storage should retain the original object. Please apply the same NaN exclusion to those unboxed float paths before switching is_w/id to address identity.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/7cbda573ead44617d1086feec3ee2fdf5094292f/pyre-interpreter/src/baseobjspace.rs#L4149-L4150
Keep NaNs boxed in the hot empty-list append path
When a hot loop creates an empty list and appends a NaN, this pointer-identity rule requires retaining the original float object, but try_walker_specialize_list_append in pyre-jit-trace/src/jitcode_dispatch/specialize.rs:9880-9913 still classifies every exact float as ListStrategy::Float and emits unboxed storage, while the concrete runtime classifier rejects NaNs. The compiled path can therefore rebox the value and make items[0] is nan false (or diverge from its Object-strategy shadow). Fresh evidence beyond the existing tuple/mapdict comment is this separate hot-append classifier, which also needs the !is_nan() gate.
AGENTS.md reference: AGENTS.md:L14-L20
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
7cbda57 to
19a0772
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 19a077278d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if unsafe { pyre_object::w_float_get_value(w_value) }.is_nan() { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Decline NaN float stores in the JIT attr path
This runtime NaN guard is not mirrored in the STORE_ATTR JIT fold: try_walker_specialize_store_attr still accepts an existing unboxed-float slot with only is_float(concrete_value) before emitting jit_mapdict_unboxed_write_f, so a traced c.x = nan after c.x was initialized with a finite float writes raw NaN bits instead of taking plain_direct_write's conversion-to-boxed path. Fresh evidence is that the fold's value gate is still is_float, not this value_has_unbox_type predicate; future reads then rebox the slot, making c.x is nan and id(c.x) diverge under JIT.
AGENTS.md reference: AGENTS.md:L14-L19
Useful? React with 👍 / 👎.
19a0772 to
aabf2a8
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aabf2a8af9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| /// `item` must be null or point to a live object. | ||
| #[inline] | ||
| unsafe fn is_float_strategy_item(item: PyObjectRef) -> bool { | ||
| pub unsafe fn is_float_strategy_item(item: PyObjectRef) -> bool { |
There was a problem hiding this comment.
Use the NaN-aware list gate for traced setitem
With this patch's NaN pointer identity, rejecting NaNs from Float storage becomes semantic, but the traced STORE_SUBSCR paths still do not share this exported predicate: trace_helpers/typed_trace.rs::detect_list_setitem_strategy and specialize.rs::try_walker_specialize_store_subscr both gate float-list writes with is_float before emitting a raw float-block store. In a traced lst = [1.0]; n = float('nan'); lst[0] = n, the interpreter converts the list to Object storage, while the compiled path leaves a Float list containing only the NaN bits, so the next read reboxes and lst[0] is n/id(lst[0]) == id(n) diverge. Please route those setitem gates through is_float_strategy_item as well.
AGENTS.md reference: AGENTS.md:L14-L19
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pyre/pyre-object/src/tupleobject.rs`:
- Around line 492-499: Validate the makespecialisedtuple2 JIT-visible change by
running cargo check --features dynasm and cargo test --features dynasm, then run
all eight benchmarks and investigate and explain any performance regressions
before committing.
- Around line 509-527: Add regression tests covering NaN operands through
makespecialisedtuple2 and w_tuple_new. Assert the resulting tuple does not use
the _ff specialization and that w_tuple_getitem returns the original NaN
PyObjectRef, while preserving the existing finite-pair test to verify the direct
_ff path.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 584d9d2a-b13a-4c4e-80f4-ed91014b004e
📒 Files selected for processing (2)
pyre/pyre-interpreter/src/objspace/std/mapdict.rspyre/pyre-object/src/tupleobject.rs
aabf2a8 to
6aa3d35
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
6aa3d35 to
8f4208a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rs`:
- Line 4020: Add a walker_guard_exact_w_class check for the canonical FLOAT_TYPE
immediately before walker_guard_float_not_nan in the relevant specialization
path. Ensure the guard’s side exit uses the generic mapdict write and performs
boxed-storage conversion for float subclasses, preserving is_unboxable_float
requirements.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2a24cc04-bdd4-4cdd-a6e1-d98c92395d25
📒 Files selected for processing (7)
pyre/extra_tests/parity_tests/bool_text_signatures_python314.pypyre/extra_tests/parity_tests/float_subclass_unboxed_storage.pypyre/extra_tests/parity_tests/nan_unboxed_storage_identity.pypyre/pyre-interpreter/src/baseobjspace.rspyre/pyre-jit-trace/src/jitcode_dispatch/mod.rspyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rspyre/pyre-jit-trace/src/trace_helpers/typed_trace.rs
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/b8f948290dc3bd1590b992648bb7c6babf9c591f/pyre-jit-trace/src/jitcode_dispatch/specialize.rs#L4016-L4020
Guard float subclass STORE_ATTR replays
When a trace is recorded for an exact finite-float assignment to an existing unboxed mapdict slot, this path declines float subclasses only at record time. On replay, walker_unbox_float guards only ob_type == FLOAT_TYPE, and a float subclass shares that ob_type while carrying a different w_class; the new non-NaN guard still passes for finite subclass instances, so jit_mapdict_unboxed_write_f stores raw f64 instead of taking _direct_write's convert-to-boxed path. A later c.x reboxes as an exact float, making c.x is subclass_value and type(c.x) wrong; add the same w_class pin used by the list float-store paths before emitting the raw write.
AGENTS.md reference: AGENTS.md:L14-L19
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Assisted-by: codex-5.6-sol
Assisted-By: Claude Opus 5
b8f9482 to
483c426
Compare
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/483c426dd7c63256e01e8c594a873fbab9f836d2/pyre-jit-trace/src/jitcode_dispatch/specialize.rs#L4020
Guard w_class before unboxed attr float stores
When an existing mapdict slot is unboxed-float, this added NaN guard still only protects the raw payload after walker_unbox_float, whose replay guard pins ob_type but not the Python w_class. In a traced loop like obj.x = v where the trace is recorded with exact finite floats and later v is a finite float subclass, the subclass shares FLOAT_TYPE, passes raw != raw as non-NaN, and jit_mapdict_unboxed_write_f stores only the f64; the interpreter path now says subclasses convert the slot to boxed storage, so a later obj.x is v/type(obj.x) diverges. Please add the same exact-w_class guard used by the list float paths before taking this helper.
AGENTS.md reference: AGENTS.md:L14-L19
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Cherry-picked from #1144, minus two hunks. The `pub(crate)` bump on `walker_exact_builtin_class` is dropped: `specialize` is a child module of `jitcode_dispatch`, so the private declaration is already in scope at every call site. The `trace_helpers/typed_trace.rs` hunk is dropped with the file, which #1318 deleted. Assisted-By: Claude Opus 5
Cherry-picked from #1144, minus two hunks. The `pub(crate)` bump on `walker_exact_builtin_class` is dropped: `specialize` is a child module of `jitcode_dispatch`, so the private declaration is already in scope at every call site. The `trace_helpers/typed_trace.rs` hunk is dropped with the file, which #1318 deleted. Assisted-By: Claude Opus 5
Cherry-picked from #1144, minus two hunks. The `pub(crate)` bump on `walker_exact_builtin_class` is dropped: `specialize` is a child module of `jitcode_dispatch`, so the private declaration is already in scope at every call site. The `trace_helpers/typed_trace.rs` hunk is dropped with the file, which #1318 deleted. Assisted-By: Claude Opus 5
Cherry-picked from #1144, minus two hunks. The `pub(crate)` bump on `walker_exact_builtin_class` is dropped: `specialize` is a child module of `jitcode_dispatch`, so the private declaration is already in scope at every call site. The `trace_helpers/typed_trace.rs` hunk is dropped with the file, which #1318 deleted. Assisted-By: Claude Opus 5
…e (27 functions), plus isclose/comb/perm (#1378) * objspace: NaN and complex take Python 3.14 pointer identity Assisted-by: codex-5.6-sol * jit: pin w_class on the float list-store fast paths Cherry-picked from #1144, minus two hunks. The `pub(crate)` bump on `walker_exact_builtin_class` is dropped: `specialize` is a child module of `jitcode_dispatch`, so the private declaration is already in scope at every call site. The `trace_helpers/typed_trace.rs` hunk is dropped with the file, which #1318 deleted. Assisted-By: Claude Opus 5 * math: call the __floor__/__ceil__/__trunc__ descriptor without binding it `math_unary_int` resolved the dunder with `lookup_special`, which binds the descriptor through `get` and returns a bound method that `call_function` then unwraps. `interp_math.py:393 floor`, `:496 ceil` and `:59 trunc` instead take `space.lookup` + `space.get_and_call_function`, which calls the unbound descriptor with the object leading the positionals; pyre has both halves already. A descriptor whose `__get__` raises still propagates, because `get_and_call_function` binds through `get` for everything except a function or method descriptor. Assisted-by: Claude * math: reduce floor/ceil's __float__ fallback through newlong_from_float The fallback boxed `v.floor() as i64`. Rust's float-to-int cast saturates, so `math.floor(FloatLike(1e300))` answered `i64::MAX` instead of the exact integer, `math.floor(FloatLike(nan))` answered `0` instead of raising ValueError, and an infinite operand answered a machine bound instead of raising OverflowError. CPython 3.14 and pypy3 7.3.20 agree on all eight cases. `float_to_pyint` already implements `newlong_from_float`; route the fallback through it. It also gains the `ovfcheck_float_to_int` arm that `floatobject.py:151-158 newint_from_float` tries before materialising a long, so an in-range value no longer allocates a BigInt to immediately discard. Assisted-by: Claude * math: reduce a machine-word gcd pair without rbigint `gcd` folded every argument through `get_bigint`, so reducing two machine words allocated an `RBigIntGcRoot` box plus five digit blocks and ran a divmod. `interp_math.py:747 gcd_two` reads both operands as Signed and only replays in the rbigint domain when one overflows; `gcd_binary` is already ported, so expose it and take the same arm. `checked_abs` is the overflow direction, so `i64::MIN` still reaches rbigint. Assisted-by: Claude * jit: read a plain residual builtin call's positionals from the shadow slots `bh_call_fn_impl` built a `Vec` per residual call through `reload_args`. The bound-receiver arm just above already reads an exactly-arity-matched builtin's positionals out of a stack array; extend the same shape to a call with no bound receiver and at most four positionals. The slice contents are identical, so `builtin_code_call_positional` sees no change. Assisted-by: Claude * jit: specialize math.floor, math.ceil, math.trunc and math.fabs All four kept the opaque `bh_call_fn` residual, so a hot loop paid the whole interpreter body every iteration: the rounding trio looked the dunder up on the argument's type and called it, and `fabs` re-entered the arity wrapper for one sign mask. `try_walker_specialize_math_round_to_int` recreates what `interp_math.py:393` / `:496` / `:59` do for an exact float — the type's own reduction followed by `newint_from_float`, whose `ovfcheck_float_to_int` arm is a machine cast. It unboxes the operand, guards it into the signed range, rounds, and casts. `floor` and `ceil` emit a pure elidable `CALL_F`; `trunc` needs none, because `CastFloatToInt` already truncates toward zero. The range guard sits on the operand rather than the rounded value, which covers all three modes: `-2**63` is an integer, `|trunc(x)| <= |x|`, and every float below `2**63` large enough for `ceil` to move it is already integral. `try_walker_specialize_math_fabs` emits one `FloatAbs` and carries no domain guard, `fabs` being total. An int argument, a float subclass, NaN, either infinity, an operand outside the signed range and a rebound callable all keep the residual. Assisted-by: Claude * bench(synth): record the wasm jit-stats baselines for the two math fold fixtures A synthetic fixture without a per-backend baseline is a red "jit-stats baseline missing" on the leg that runs it, and the wasm leg has no exemption header for these two. Both compile one loop and no bridge, matching the dynasm and cranelift baselines. Assisted-by: Claude * math: fold every remaining pymath primitive through a raw helper table Add `MATH_FLOAT1_FOLDS` / `MATH_FLOAT2_FOLDS`, mapping each `math` builtin's checked-arity wrapper pointer to a raw helper that makes the same `pymath` call the builtin body makes and reports every error direction as NaN. The walker guards the result finite, so a helper answer that is finite is the value the builtin returns; a NaN resumes in the builtin, which raises or returns the non-finite value itself. Covers tan, asin, acos, atan, sinh, cosh, tanh, asinh, acosh, atanh, cbrt, exp, exp2, expm1, log1p, erf, erfc, gamma, lgamma, ulp, degrees, radians, pow, fmod, copysign, remainder and atan2. sqrt, log, cos, sin and fabs keep their dedicated specializations, which lower to tighter shapes. `jit_math_isclose_default` spells out the comparison for the both-tolerances- defaulted form rather than delegating, so it is total and its answer can be read as a plain truth value. comb and perm gain machine-word arms: `get_bigint` allocates a digit block per operand before the reduction allocates another per multiplication, and a pair of machine ints answers the same value with neither. Each comb step is the exact `C(n, i-1) * (n - i + 1) / i`, so the running value is a real binomial coefficient throughout; an intermediate that leaves the range replays the pair in the rbigint domain. Assisted-by: Claude * jit: specialize the generic math float folds and math.isclose `try_walker_specialize_math_float{1,2}` replace the opaque `bh_call_fn(builtin, NULL, x[, y])` residual with the unboxed operands, one pure elidable `CALL_F` into the function's raw helper, a finite-result guard and an inline `wrapfloat`. The guard is what carries the domain: the helper reports every raising direction as NaN, so the fold needs no per-function domain knowledge and adding a function to the interpreter's table is all it takes to cover it. `try_walker_specialize_math_isclose` folds the both-tolerances-defaulted form where the result decides one branch and nothing else, so the branch's own guard stands in for the box and the fold carries no result guard. It settles that shape before emitting anything, and compares the helper's answer against the interpreter's on the recorded operands before committing. The fold suppression mask moves from a single `u64` to `SpecMask`, one bit per `SPEC_FOLD_ROWS` entry: the table reached 63 rows and `1u64 << 64` is not a mask this could keep growing into. Assisted-by: Claude * bench(synth): merge the four math fold fixtures into one `math_log_trig_hot`, `math_fabs_hot` and `math_round_to_int_hot` become `math_folds_hot`, one loop per fold shape, plus loops for the generic float folds and for `isclose`. `math_sqrt_hot` stays where it is: it now gates `math.isqrt` as well as `math.sqrt`, against a ceiling fitted to its own two measured states, and this branch touches neither fold. The ratio is this fixture's only detector: losing a fold changes no jit-stats counter, because the residual it falls back to compiles the same loop. At load 11 on darwin-arm64, against pypy 0.33s, it runs 0.63s with every fold, 2.71s with the generic float and isclose folds suppressed and 33.4s with all folds suppressed, so `max-pypy-ratio` is set at 5, between the first two. `max-wasm-ratio` is fitted to 8.1-9.0x across five runs plus the 11.3x seen during a load spike, +15%. wasm is slower here for a structural reason: on the same fold machinery and the same loop it runs 2M folded `log` (which lowers to `x.ln()`) in 0.09s and 2M folded `exp` (which goes through `pymath`) in 0.24s, because `pymath` reaches the platform libm on native and its pure-Rust fallback in the guest. stdlib_math.py runs each covered function hot on one operand at a time, so the loop compiles and whichever of the fold or the decline it chose runs for every iteration, and checks the answer against the one the interpreter gave before anything was compiled — over the folded domain, the boundaries where the guard hands the call back, and the raising directions. Assisted-by: Claude * jit: read a residual call's roots through the scope's cached cell bh_call_fn_impl opened a RootScope and then reached for the free gc_roots::pin_root / shadow_stack_get / shadow_stack_len functions for all nineteen of its shadow-stack accesses. Each of those resolves the thread-local again; RootScope already holds the resolved cell for exactly this reason. Every bh_call_fn arity funnels through this one function. Assisted-by: Claude * interpreter, jit: fold a table of builtins out of their residual call A builtin without a walker specialization reaches the interpreter as bh_call_fn(builtin, NULL, args), which forces the frame, roots the arguments, resolves the execution context and binds the gateway signature before the body runs. Measured per call against pypy 7.3.20 on darwin-arm64, that leaves every unspecialized builtin between 25ns (callable) and 985ns (set(iterable)), while the operations the walker already folds -- a Python call, a list store, `is`, an attribute read -- run in 1.5 to 15ns. jit_builtin_folds names, per builtin, a raw helper carrying that builtin's body restricted to the operands it answers without running app-level code and without allocating, and reporting every other direction through its channel's decline sentinel -- i64::MIN, NaN, or PY_NULL. The walker emits a direct call into the helper, the guard that reads the sentinel, and an inline wrapint / wrapfloat the optimizer can keep virtual; a decline resumes in the builtin, which re-executes the call. Adding a table row is therefore all it takes to cover another builtin. The first rows are hash, ord, abs (one row per result channel), min and max. Per call, they move into the folded band: abs(int) 4.4ns abs(float) 3.4ns ord(c) 4.8ns hash(int) 6.4ns hash(str) 6.7ns min/max 3.0 / 3.1ns and abs's compiled loop goes from 45 ops / 12 guards carrying a CallMayForceR to 37 ops / 10 guards carrying a CallI. Nothing here allocates: a reference-returning helper would leave the result allocation in place, which a sample profile puts at a third of the residual's cost, so the scalar channels are what reach this band. Every helper is spelled extern "C" fn(i64, ...) and casts at its own boundary. The wasm backend lowers an all-Int/Ref residual to a direct call_indirect whose type is (i64 x n) -> i64, fabricated from the descr's arity alone; a PyObjectRef parameter is an i32 on wasm32, so a helper spelled with pointer arguments traps the moment a compiled trace calls it. The scalar channels are emitted under CANNOT_RAISE_NO_HEAP_EFFECT_INFO, whose can_collect is false and therefore carries no gcmap and spills no reference registers. hash declines on a NaN float for that reason rather than for its answer: hash_value routes a NaN to the identity hash, and a float's identity widens its bit pattern into a fresh int. Assisted-by: Claude * bench(synth): add the builtin fold fixture and its native jit-stats baselines Six loops, one per folded row -- hash(int), hash(str), ord, abs(int), abs(float) and min/max -- each long enough to compile. The fixture prints only deterministic values, so hash(str) counts iterations agreeing with the first digest rather than summing a seed-randomized one. Read from check.py itself on darwin-arm64 at load 18: 5.0x, 4.4x and 4.5x with the folds in place, 52.9x and 61.4x with PYRE_FBW_NO_SPECIALIZE=builtin_fold1,builtin_fold2 putting the same loops back on the residual. The header gate sits at 8x, 60% above the first arm and more than six times below the second. Assisted-by: Claude * jit: decline a fold before it runs the builtin, and cross-check every float helper Two orderings the fold specializers had wrong. `try_walker_specialize_builtin_fold1` / `_fold2` executed the builtin to get the authentic result and only then asked the raw helpers, so an operand no row answers for -- an object with a Python `__hash__`, an `int` subclass carrying `__abs__` -- ran the builtin once for the walk and once more in the residual the decline falls back to, observable twice in a single walk. The helpers are asked first, and the builtin runs only once some row has answered. `try_walker_specialize_math_float1` / `_float2` recorded the builtin's answer as the concrete for a `CALL_F` into the raw helper without ever comparing the two, so a helper that disagreed with the function it stands for compiled that disagreement into the loop. Both now compare, and by bit pattern rather than `==`, which cannot tell `-0.0` from `0.0` -- a difference `copysign` observes. `try_walker_specialize_builtin_fold1`'s float arm compared with `==` and now compares the same way. `try_walker_specialize_math_fabs` read `boxed_result`'s float payload without checking the box, and read it after recording the callable guard. It now rejects a non-float result and compares `FloatAbs` over the coerced operand against the builtin's answer, both before anything is recorded, so a decline leaves no guard behind. Assisted-by: Claude * jit: give the two-argument builtin fold its result value before it guards `try_walker_specialize_builtin_fold2` emitted `GuardNonnull` over the call result and only then stamped that result's concrete, so the resume snapshot the guard captures recorded an OpRef with no value. The one-argument half already stamps first; this half now matches. The same call carried `EffectInfo::new(CannotRaise, OopSpecIndex::None)`, whose `can_collect` is true and therefore asks every backend for a spill / gcmap / reload bracket around it. `min` and `max` compare two exact scalars and return one of their own arguments, so the call cannot collect and now says so through `CANNOT_RAISE_NO_HEAP_EFFECT_INFO`. Assisted-by: Claude * math: convert isclose's operands before checking the tolerances `interp_math.py:698-705` converts a, b, rel_tol and abs_tol in that order and only then rejects a negative tolerance, so an operand that is not a number is reported even when a tolerance is also rejectable. pyre read the tolerances first, so `math.isclose("x", 1.0, rel_tol=-1)` raised ValueError where CPython 3.14 and PyPy 7.3.20 both raise TypeError, and a user `__float__` on the operands ran after the one on the tolerances. The snippet pins both the exception and the conversion order, and adds the keyword rejection for comb/perm/gcd/lcm. Assisted-by: Claude * bench(snippets): make the min/max tie assertion observable `_a, _b = 10**3, 10**3` binds one object on both CPython and PyPy — the constant is folded and deduped in co_consts — so `min(_a, _b) is _a` held whichever operand the fold returned. `_stable` also reports the answer it computed before its loop, so the assertion never read a folded value at all. Signed zeros are the tie whose operands stay distinguishable: `is` on two exact ints compares values, so no equal int pair can witness this, while two exact floats compare bit patterns. Read the identity inside the loop, through the plain two-argument call shape the specializer matches. Passes on CPython 3.14.2 and PyPy 7.3.20. Assisted-by: Claude * jit: keep the min/max fold off bigint operands `is_exact_type` answers on `w_class`, and `w_long_from_raw` wires a bigint's `w_class` to `int`'s so that `type(x) is int` holds for one. `compare_pair` gated on that alone, so a `W_LongObject` took the machine-int arm and `w_int_get_value` read its `value: *mut BigInt` from the offset `W_IntObject` keeps `intval` at -- the comparison ran on the payload's heap address. `int` is the only type in the fold table with two layouts behind one `w_class`: the census of `w_class: get_instantiate(&...)` shows `INT_TYPE` written by both `intobject.rs` and `longobject.rs`, while `FLOAT_TYPE`, `STR_TYPE` and `BYTES_TYPE` each have one layout. Add the `is_int` conjunct, which reads `ob_type` and still separates them -- the same pair the dict's builtin-key test uses. An address is always a large positive number, so the existing `(2**70, 1)` case agreed by accident; the answer only diverges once the bigint is the operand that should lose. The fixture now covers that direction. Assisted-by: Claude * bench(snippets): cover the bigint pair whose address order is deterministic `(2**62, 2**62 + 1)` reaches no bigint at all -- both fit a machine word -- and a payload address sits far below 2**62, so `(2**70, 2**62)` diverges under the misread whichever way the allocator places it. Assisted-by: Claude * jit: keep the abs fold off int subclasses `is_int` reads `ob_type`, which a subclass instance shares with the builtin, so it alone answered for an `int` subclass -- and the fold emits no operand-class guard, so a compiled loop recorded with a plain `int` went on answering after one arrived carrying an `__abs__` override. `is_exact_type` reads `w_class`, which the subclass retags. Neither test implies the other and both are needed: `is_exact_type` alone would admit a bigint, whose `*mut BigInt` sits where `intval` does. It also subsumes the `bool` rejection, whose own arm sits above. Measured before this change, on a loop over an `int` subclass whose `__abs__` returns a string: the fold answered with the payload's absolute value. Assisted-by: Claude * builtins: keep bigints out of the all-int sorter `sort_compare_for` stands in for the integer list strategy, so it must accept exactly what that strategy does. It gated on `is_exact_type` against `INT_TYPE` alone, which answers on `w_class` -- and a bigint's is wired to `int`'s so that `type(x) is int` holds for one. A list holding a bigint therefore classified as all-int and sorted through `int_value`, which reads the `*mut BigInt` from the offset a machine int keeps `intval` at. Measured: `sorted([-(2**70), 5])` answered `[5, -1180591620717411303424]`. `is_plain_int1` is the strategy's own `is_correct_type` and carries both halves. A payload address is always a large positive number, so only a bigint that should lose to the other operand tells the two orders apart. Assisted-by: Claude * builtins: dispatch abs() through the receiver's __abs__ `builtin_abs_obj` answered from the int/long/float/complex layout arms before it looked for `__abs__`, so a subtype that replaced the builtin one -- `__abs__ = None` included -- got the structural answer instead of its own. Split the layout arms out as `abs_structural` and gate them on `abs_uses_builtin`, the shape `round_uses_builtin` already carries for `__round__`; anything else dispatches through the type. `int.__abs__` and `float.__abs__` now name `builtin_abs_dunder`, which is `abs_structural` alone, so an override that delegates back to the slot does not re-enter the lookup that reached it. `builtin_abs.py` covers the five cases; it fails on the previous binary at its first assertion. Assisted-by: Claude * comments: cite this branch's upstream references by symbol `check-new-line-citations.py --base origin/main` flags the eight `file.py:LINE` citations this branch adds. Each now names the enclosing upstream symbol: `floor`, `ceil`, `trunc`, `fabs`, `isclose`, `gcd_two`, and `newint_from_float`. Assisted-by: Claude * builtins: give int's __float__ and the __round__ slots a structural body Two defects the `abs()` dispatch fix names but does not reach. `float()` converted an `int` from its layout before it looked `__float__` up, so an `int` subtype's override was ignored -- `float(S(-5))` returned -5.0 where both runtimes raise. The `float` arm beside it already fell through to the lookup for exactly this reason; the `int`, `bool` and long arms now gate on `is_exact_type` the same way. That lookup resolves to `int.__float__` when the subtype does not override it, so that slot gets a structural body, `builtin_int_float_dunder`, mirroring the `float`-side `builtin_float_dunder` whose doc already states the rule. `number_dunder_round` forwarded to the dispatching `builtin_round`, so a subtype whose `__round__` calls `int.__round__(self)` re-entered the lookup that reached it: `RecursionError` where both runtimes answer -5. The body is now `round_receiver(args, slot)`, and the slot both forces the structural arms and skips the trailing lookup. Assisted-by: Claude * portal: key the pypyjit green on the running profile state `pypyjit_greenkey`/`pypyjit_greenkey_uhash` already carried `is_being_profiled` as a parameter; every production caller passed a literal `false`, which the two green-key helpers documented as a parity gap against `interp_jit.py`'s `greens = ['next_instr', 'is_being_profiled', 'pycode']`. Both the hash form and the typed form now derive it from `current_is_being_profiled`, which reads `profilefunc` off the running execution context. Deriving it inside the helpers rather than at the call sites is what keeps the two forms naming one cell: a function entry keys on `(pycode, 0)` with no frame in hand, and `JitCell.comparekey` cannot find a cell filed under a different green tuple. `setllprofile` sets the per-frame flag on every live frame (`force_all_frames(is_being_profiled=True)`) and `call_trace` sets it on each frame it enters, so the frame flag and "a profile function is installed" name the same state for every frame the portal reaches. The `eval.rs` gate that sends a profiled frame to the plain evaluator is unchanged, so no profiled frame reaches the portal yet; its comment now records what was measured when the gate was narrowed. Assisted-by: Claude * check: band the collection-schedule guard_failures on inline_freevar_after_mayforce `guard_failures` on this fixture counts each guard's warm-up against the collection schedule rather than a compile decision. One binary swept across nursery sizes read 1034 / 1014 / 1007 / 1007 at 2 / 4 / 6 / 8 MB while `loops_compiled` and `bridges_compiled` did not move; suppressing the whole trace-time fold table moved it by one count and suppressing the folds this branch adds by none. Against the recorded baselines the three CI runners read 1011 on cranelift and darwin-arm64 reads 1012 across three consecutive gated runs, with dynasm at 1005 against 1004. Band `guard_failures` at width 8, matching the width `generator_tree_recursion` already carries, and leave the compile counters gated exactly. The header claimed every gated counter is independent of N past 48000 and named six loops with a cranelift value of 1010; the recorded baselines hold seven loops and 1008. Restate the claim as the compile decisions. Assisted-by: Claude * check: record builtin_folds_hot's wasm baseline and refit both its ceilings The fixture had no `.wasm.jitstats`, so the ubuntu leg failed the baseline check, and its wasm/dynasm ratio of 8.2x failed the 3.5x global ceiling. Record the wasm baseline -- it reads the same counters as dynasm and cranelift, six loops and six guard failures with no bridges -- and state `max-wasm-ratio=10`, fitted to the highest reading observed plus 15%: 8.2x on ubuntu-24.04 and 8.7x on darwin-arm64. Two architectures under two load regimes land within half a count of each other. The header names the structure behind it: a JIT-emitted trace is its own wasm module, so a call leaving it crosses back through the `env.jit_call` trampoline, and every fold here still lowers to a call. `math_folds_hot`, whose folds lower to inline arithmetic, reads 3.3x on the same ubuntu run. Raise `max-pypy-ratio` from 8 to 12. With every fold in place the three runners read 4.6x/4.7x, 7.2x/7.6x and 9.2x/10.0x; the windows pair cleared 8 only through `_compare_buffer`, which is two timer quanta per unit of limit there. Add `spec-folds=builtin_fold1,builtin_fold2`, which gates each fold's coverage directly rather than leaving the summed ratio as the only detector of a lost fold. Both labels fire here, 5 and 2. Assisted-by: Claude * check: state a wasm ceiling for str_getitem_len_hot, and correct the constant's comment The fixture reads over the 3.5x wasm/dynasm ceiling on every branch that measures it, not only on this one: a census of eleven branch runs on 2026-08-22 read 3.6x six times, 3.7x twice, 3.8x once and 4.1x once, with the one remaining run not reaching the leg. Set `max-wasm-ratio=4.8`, the highest reading plus 15%, and say in the header that it is an allowance and not a fix -- the leg is a residual STRGETITEM/UNICODEGETITEM loop, so on wasm every iteration crosses out of the trace module through `env.jit_call`. `WASM_MAX_DYNASM_RATIO`'s comment still ended "No fixture carries an allowance today". Three do. Name them and the structure they share. Assisted-by: Claude * check: double builtin_folds_hot's loop counts and refit its wasm ceiling to 13 Two ubuntu-24.04 runs of the same code read 8.2x and 11.3x wasm/dynasm. The denominator is what moved: dynasm's execution-only time came out 0.69s and then 0.44s, and the failing run's own detail line said a dynasm startup estimate 0.68x larger would have erased the gap. The startup subtraction's error is a fixed number of milliseconds, so doubling HASH_N/ORD_N/ABS_N/MINMAX_N halves its share of both sides. Every recorded jit-stats counter is unchanged by the doubling -- dynasm and cranelift both still read six loops, six guard failures, no bridges and no aborts -- so no baseline is re-recorded. Set `max-wasm-ratio=13`, the highest reading observed plus 15%. The doubling also carries the windows pypy baseline over FLOOR_GATE_MIN_BASELINE_S. It sat under it at the previous counts, which is why that runner's ratios printed with a `?`, and the pair cleared the ceiling of 8 the fixture carried then only because `_compare_buffer` grants two timer ticks per unit of limit on that platform. Assisted-by: Claude --------- Co-authored-by: kyokuping <me@kyoku.dev>
|
I put these commits into #1378, thanks! |
Assisted-by: codex-5.6-sol
Summary
The NaN comparison cases in builtin_tuple, builtin_list, and builtin_slice exposed that pyre was still inheriting PyPy's value-identity behavior.
Align
is_w/ identity semantics for NaN floats and complex numbers with Python 3.14 pointer-identity behavior.is_w: NaN is never identical to a distinct object; finite floats retain bit-pattern identity (unboxed in FloatListStrategy, reboxed on read).is_w: dropped the complex branch entirely — nothing stores complex unboxed, so pointer identity is free and matches 3.14 forcomplex(1,2) is complex(1,2).immutable_unique_id: no value-derived id for NaN floats or any complex, preservingx is y <=> id(x) == id(y);hash()follows for free.Prevented
NaNs from entering identity-erasing unboxed storage inFloatListStrategy, specialized float tuples, andmapdictattributes, preserving the original object across reads.Self-review
Assisted-bySummary by CodeRabbit
Bug Fixes
floatsubclasses during optimized storage and JIT execution.Tests